CORS-4425: gcp: add OS image validation for sovereign clouds#10709
CORS-4425: gcp: add OS image validation for sovereign clouds#10709barbacbd wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@barbacbd: This pull request references CORS-4425 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesSovereign OS image validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/types/gcp/validation/machinepool_test.go`:
- Around line 73-80: Update the table-driven test around
ValidateOSImageForSovereignCloud to retain the field.ErrorList instead of only
converting it to an aggregate error, then assert that invalid cases report the
expected test-path.osImage field path, including nested name or project paths
where applicable. Keep the existing valid-case no-error assertion.
In `@pkg/types/gcp/validation/machinepool.go`:
- Around line 91-92: Update the validation logic surrounding the osImage check
in machine-pool validation so a non-nil but incomplete image reports missing
fields at osImage.name and/or osImage.project, while retaining the parent
osImage required error only when the image object is nil. Ensure each error path
matches the user-facing YAML/JSON structure and remains actionable.
- Around line 90-92: Update the OS image validation condition in the machine
pool validation flow to treat a non-nil pool.OSImage as the selected override,
validating its Name and Project without falling back to
platform.DefaultMachinePlatform.OSImage. Consult the default image only when
pool.OSImage is nil, and add a regression test covering an incomplete pool image
with a complete default image.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 27d610fa-2339-4fda-ae5a-853a08259a24
📒 Files selected for processing (3)
pkg/types/gcp/validation/machinepool.gopkg/types/gcp/validation/machinepool_test.gopkg/types/validation/machinepools.go
| func ValidateOSImageForSovereignCloud(platform *gcp.Platform, pool *gcp.MachinePool, fldPath *field.Path) field.ErrorList { | ||
| allErrs := field.ErrorList{} | ||
|
|
||
| if !strings.Contains(platform.ProjectID, ":") { |
There was a problem hiding this comment.
We can re-use an existing func, right?
installer/pkg/types/gcp/platform.go
Lines 220 to 224 in d8f6a96
| } | ||
|
|
||
| func hasValidOSImage(pool *gcp.MachinePool) bool { | ||
| return pool != nil && pool.OSImage != nil && pool.OSImage.Name != "" && pool.OSImage.Project != "" |
There was a problem hiding this comment.
💡 We should validate and report individual missing fields to make it clearer for users, similar to what coderrabit recommended: #10709 (comment). How about?
if pool == nil || pool.OSImage == nil {
allErrs = append(allErrs, field.Required(fldPath.Child("osImage"),
"must specify an OS image for sovereign cloud environments (domain-scoped project ID)"))
return allErrs
}
osImagePath := fldPath.Child("osImage")
if pool.OSImage.Name == "" {
allErrs = append(allErrs, field.Required(osImagePath.Child("name"),
"must specify an OS image name for sovereign cloud environments"))
}
if pool.OSImage.Project == "" {
allErrs = append(allErrs, field.Required(osImagePath.Child("project"),
"must specify an OS image project for sovereign cloud environments"))
}| return allErrs | ||
| } | ||
|
|
||
| if !hasValidOSImage(pool) && !hasValidOSImage(platform.DefaultMachinePlatform) { |
There was a problem hiding this comment.
I think, by this point, the pools would already merge with platform.defaultMachinePlatform, so we don't have to check it.
installer/pkg/types/defaults/machinepools.go
Lines 72 to 73 in d8f6a96
| allErrs = append(allErrs, gcpvalidation.ValidateMachinePool(platform.GCP, p.GCP, f)...) | ||
| allErrs = append(allErrs, gcpvalidation.ValidateMasterDiskType(pool, f)...) | ||
| allErrs = append(allErrs, gcpvalidation.ValidateServiceAccount(platform.GCP, pool, f)...) | ||
| allErrs = append(allErrs, gcpvalidation.ValidateOSImageForSovereignCloud(platform.GCP, p.GCP, f)...) |
There was a problem hiding this comment.
I think this line is only called if the pool defines a non-nil platform.gcp field, according to:
installer/pkg/types/validation/machinepools.go
Lines 165 to 167 in d8f6a96
That said, a minimal install-config.yaml like below will skip the validation completely.
compute:
- architecture: amd64
hyperthreading: Enabled
name: worker
platform: {}
replicas: 3
controlPlane:
architecture: amd64
hyperthreading: Enabled
name: master
platform: {}
replicas: 3
platform:
gcp:
projectID: "eu0::project"How about running the osImage validation as long as the install platform is GCP? This matches the AWS AMI validation pattern.
if platform.GCP != nil {
allErrs = append(allErrs, gcpvalidation.ValidateOSImageForSovereignCloud(platform.GCP, p.GCP, fldPath.Child("gcp"))...)
}
if p.GCP != nil {
validate(gcp.Name, p.GCP, func(f *field.Path) field.ErrorList { return validateGCPMachinePool(platform, p, pool, f) })
}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/types/validation/machinepools.go`:
- Around line 165-167: Update the GCP validation call in the machine-pool
platform validation flow to pass fldPath.Child("gcp") instead of appending the
duplicate "platform" segment. Add an integration test that verifies the
resulting validation error path matches the YAML/JSON structure, such as
...platform.gcp.osImage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5e8908d2-9062-4f5d-9006-c43df1a3346a
📒 Files selected for processing (3)
pkg/types/gcp/validation/machinepool.gopkg/types/gcp/validation/machinepool_test.gopkg/types/validation/machinepools.go
|
/test e2e-gcd-ovn-private-techpreview |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Sovereign cloud environments (identified by a domain-scoped project ID containing ":") require an OS image to be specified. This adds install-config validation ensuring that each compute and control plane machine pool has an OS image configured, either directly or via the default machine platform.
|
Scheduling required tests: Scheduling tests matching the |
|
@barbacbd: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Sovereign cloud environments (identified by a domain-scoped project ID containing ":") require an OS image to be specified. This adds install-config validation ensuring that each compute and control plane machine pool has an OS image configured, either directly or via the default machine platform.
Summary by CodeRabbit